Repository navigation
ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs - #101
Conversation
…rged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — embeddedos-org#93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. embeddedos-org#87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) embeddedos-org#93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — embeddedos-org#86 and embeddedos-org#57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only embeddedos-org#57's public_key_is_valid_subgroup() is wired to the call site, so embeddedos-org#86's key_has_prime_order() was dead. Kept the live function, folded embeddedos-org#86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on embeddedos-org#86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight.
Four of the five fuzz harnesses did not test what they name, and the
build could not say so because not one of them includes a project
header. Each declares its target itself:
fuzz_recovery_protocol.c extern int eos_recovery_parse_packet(...)
fuzz_fw_update.c extern int eos_fw_update_init(void)
extern int eos_fw_update_process_chunk(...)
extern int eos_fw_update_finalize(void)
fuzz_bootctl.c extern int eos_bootctl_parse(...)
fuzz_image_verify.c extern int eos_image_parse_header(const void *, size_t)
eos_recovery_parse_packet, eos_fw_update_init,
eos_fw_update_process_chunk and eos_bootctl_parse have never existed —
those names appear nowhere outside the harness that declares them, so
three of the five targets have never linked.
The other two are worse, because they do link:
- eos_fw_update_finalize exists but takes (ctx, mode), not (void).
- eos_image_parse_header takes (uint32_t addr, eos_image_header_t *),
not (const void *, size_t). fuzz_image_verify has been passing a
pointer where an address is expected and a length where an output
struct is expected, on every input, for as long as it has run.
A local extern is why none of this was caught: it tells the compiler the
symbol exists with whatever shape the harness asserts, and the mismatch
survives to link time or past it.
All five now include the real header and drive the real entry points:
fuzz_image_verify parse_header -> verify_integrity, boot-path order
fuzz_bootctl bootctl_load, then trap if load() accepted a
block validate() rejects
fuzz_fw_update begin -> write in fuzz-chosen chunk widths ->
finalize or abort
fuzz_recovery_protocol write_in_range, trapping on any accepted write
that leaves the slot or wraps, checked in wider
types that cannot
fuzz_crypto already correct; switched to the header so it
stays that way
Adds tests/fuzz/fuzz_sim_flash.h: the three harnesses that reach flash
need board ops installed, and without them they fuzz a null op table.
That the compiler now checks this is not theoretical — writing this, it
rejected EOS_UPGRADE_MODE_TEST for EOS_UPGRADE_TEST immediately, which
is exactly the error class the externs were hiding.
Verified. libFuzzer is unavailable here (libclang_rt.fuzzer_osx.a not
found), so each harness was linked against eboot_core and driven by a
standalone seed generator under ASan+UBSan:
all five compile -Wall -Wextra 5/5, 0 failures
20,000 inputs each 100,000 total, no reports
ctest 21/21 passed
Third instance of this class across these repos, after eos#50
(fuzz_devicetree naming eos_dtb_parse) and eos#110 (fuzz_ota_header
naming eos_ota_parse_header). The common factor every time is a fuzz
target declaring its own subject.
Stacked on embeddedos-org#94, without which include/eos_image.h does not compile.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The restored Fuzz Harness Build job on embeddedos-org#101 ran these harnesses under real libFuzzer for the first time and fuzz_recovery_protocol trapped within seconds. That was my bug, not the code's. The oracle computed the one-past-the-end address: end = base + offset + len; if (end > 0xFFFFFFFF) trap; The last byte a write of len bytes touches is base + offset + len - 1. eos_recovery_write_in_range() checks exactly that: if ((uint32_t)len - 1u > UINT32_MAX - (base + offset)) reject; So an accepted write whose final byte lands exactly on 0xFFFFFFFF -- base=0xFFFFFF00, offset=0, len=256 -- is legal, the function accepts it, and the old oracle trapped on it. A harness that traps on valid input reports the opposite of the truth. Verified both directions: the boundary case, fed directly -> no trap a fail-open stub accepting all input -> trap fires (exit 133) 500,000 random inputs, ASan+UBSan -> no reports libFuzzer doing precisely what it is for -- driving inputs into the one corner a hand-written oracle got wrong -- is the strongest argument yet for embeddedos-org#101's job. The guard could never have caught this; only execution under a coverage-driven fuzzer did.
embeddedos-org#99 repairs the five harnesses in tests/fuzz/. Neither it nor anything else stops the same thing happening again, because two of the conditions that produced it are still in place. **Nothing compiles them.** tests/fuzz/ is behind EBLDR_BUILD_FUZZ, which defaults OFF, and no job set it. That is the whole reason three harnesses could name functions that exist nowhere -- eos_bootctl_parse, eos_recovery_parse_packet, eos_fw_update_init and friends -- and sit there for as long as they did. A harness that is not built cannot fail to build. The fuzz-build job restores that: clang, EBLDR_BUILD_FUZZ=ON (and EBLDR_BUILD_TESTS=ON, since tests/fuzz is added from tests/CMakeLists.txt, so the option alone would build nothing and the job would pass having compiled no harness at all), then a five-second smoke run of each target. It does not fuzz; a real campaign belongs in a scheduled workflow. **The `extern` is still legal.** Every one of the four defects came from a hand-written prototype standing in for an #include -- a promise the compiler is obliged to believe and has no way to check. It is what made the fourth defect invisible even to a compiler: fuzz_image_verify declared eos_image_parse_header as (const void *, size_t) when it is (uint32_t, eos_image_header_t *), so that harness linked, ran, and fuzzed nothing. tests/unit/test_fuzz_harnesses.py refuses the pattern, and also checks that every harness is built by a CMake target, links eboot_core, and defines LLVMFuzzerTestOneInput. The two halves cover different failures: the guard is static and stops the pattern being written, the job is a compiler and catches a signature that drifts under a harness that includes the right header. Neither subsumes the other. Mutation-checked: restoring one extern prototype fails test_no_harness_declares_its_own_prototypes; deleting one add_executable fails test_every_harness_is_built. Verified: pytest tests/unit 26 passed 1 skipped, ctest 21/21. The fuzz job itself could not be run here -- Apple clang ships no libclang_rt.fuzzer, so -fsanitize=fuzzer will not link on this host. Instead each of embeddedos-org#99's five harnesses was compiled and linked against eboot_core with a stand-in driver under -fsanitize=address,undefined and run over ~20k pseudo-random inputs plus every length from 0 to 200; all five clean. That covers the symbol resolution and the harness logic, which is what the job's build step checks; it does not prove the libFuzzer link itself, which only CI can.
d8e0bbf to
e2bb1f9
Compare
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#101 "ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs"
head: e2bb1f9 author: Kartikey1306 ci: pass (25 checks, Fuzz Harness Build green)
Verdict: This is the right shape for the problem — a job that compiles what was never compiled, plus a static guard so the pattern that hid it cannot come back. Both work. I ran the guard against master's tests/fuzz/ and it fails naming all five hand-written extern declarations; against this head all five of its tests pass. Two things to fix: the guard has one obvious bypass, and half of fuzz_fw_update's inputs are discarded by the sim-flash geometry before any code under test runs.
This PR is #99 plus one commit (e2bb1f9f); the harness rewrites are reviewed under #99.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/unit/test_fuzz_harnesses.py:38–40 |
EXTERN_FUNCTION_RE anchors on ^\s*extern\s+, so it only catches declarations that spell extern. A file-scope function declaration has external linkage without it, and the compiler cannot check either one across translation units — they are the same defect. Verified: I dropped a harness containing int eos_image_parse_header(const void *flash_base, size_t flash_len); (the exact wrong signature from the original bug, minus the keyword) into tests/fuzz/ and test_no_harness_declares_its_own_prototypes passed. The module docstring at :20 claims "these tests refuse the pattern, so the sixth cannot be written", which is stronger than what the regex does. |
Drop extern\s+ from the pattern and exclude static and LLVMFuzzerTestOneInput instead — i.e. reject any file-scope function declaration ending in ; that is not static and not the entry point. Then re-run against master's tree to confirm it still catches all five, and against the bare-prototype probe. |
| 2 | Medium | tests/fuzz/fuzz_sim_flash.h:23,55 |
Slot B does not exist. FUZZ_FLASH_SIZE is 0x10000 while .slot_b_addr = 0xC000, .slot_b_size = 0x8000 spans 0xC000–0x14000, i.e. 32 KiB past the end of fuzz_flash[], so every erase or write covering it is rejected by fuzz_flash_erase/fuzz_flash_write at :35,:41. fuzz_fw_update.c:42 picks the slot from the input (selector & 1), so half the corpus dies at eos_fw_update_begin. Measured over 20,000 inputs: slot A tried 10,071, began 10,071; slot B tried 9,929, began 0. Half the fuzzing budget of that harness buys nothing, and the A/B asymmetry the harness exists to explore is never explored. |
#define FUZZ_FLASH_SIZE (128 * 1024), or shrink the slots to 0x2000 each. Either way the Fuzz Harness Build job should not report an execution count as if it were coverage — a per-harness "reached the code N times" counter would have caught this and would have caught #100's two dead harnesses too. |
| 3 | Low | tests/unit/test_fuzz_harnesses.py:89–95 |
test_every_harness_defines_the_entry_point asserts the string LLVMFuzzerTestOneInput appears anywhere in the file, comments included. Every harness in the tree mentions it in prose, so a file that only discusses the entry point without defining it would pass. |
Match a definition: re.search(r'\bLLVMFuzzerTestOneInput\s*\([^)]*\)\s*\{', text). |
| 4 | Low | .github/workflows/ci.yml (fuzz-build, smoke-run step) |
-max_total_time=5 with no -seed makes this a nondeterministic required check: whether it goes red depends on what coverage-guided search finds in five seconds on that runner. That is exactly how this job earned its place — it found the real off-by-one in #99's recovery oracle in six seconds, which 500k random local inputs never hit — but as a per-PR gate it means an unrelated PR can be blocked by a pre-existing defect discovered that day, with no reproducible run behind it. |
Keep the build on every PR; pin the smoke run (-seed=1 -runs=100000) and put the open-ended campaign in nightly.yml. Also add fuzz-build to #90's CI Gate needs: list — #90's test_gate_covers_every_job_that_runs_on_a_pull_request will fail loudly otherwise, which is the guard working, but whichever of #90/#101 lands second has to do it. |
Worth keeping exactly as written, because these are the two ways this job could have become a check that cannot fail and both are closed: EBLDR_BUILD_TESTS=ON alongside EBLDR_BUILD_FUZZ=ON (without it tests/fuzz/ is never added from tests/CMakeLists.txt and the job passes having compiled nothing), and the explicit ${#harnesses[@]} -eq 0 failure with nullglob. test_every_harness_is_built and test_every_harness_links_the_library_under_test cover the CMake side of the same question statically, and test_there_are_harnesses_to_check stops the glob-matched-nothing vacuity — that last one is the check most suites in this position forget.
Architecture conformance
Conforms. Tier 1 Foundation (§21); the change is .github/, tests/fuzz/ and tests/unit/, with no runtime dependency and no include pointing up a tier (§5.1). This is the concrete mechanism behind .ai/security.md's "every externally reachable parser … gets fuzz coverage, not just unit tests" and its ctest --no-tests=error rule generalised: a suite that silently collected nothing is a failed check reported as green, and this PR closes that for tests/fuzz/ in two independent ways (a job that compiles, and a static guard that runs without clang or cmake). §8's "trusted computing base minimal and auditable" is the reason it belongs in eBoot rather than in a shared CI template.
Proposed changes
- Widen
EXTERN_FUNCTION_REto any non-staticfile-scope function declaration, and re-run it against master'stests/fuzz/and against a bare-prototype probe. FUZZ_FLASH_SIZE (128 * 1024)so slot B is real, then re-measurefuzz_fw_update's per-slotbegincounts.- Match a definition, not a mention, in
test_every_harness_defines_the_entry_point. - Pin the smoke-run seed; add
fuzz-buildto #90's gate. - Land with #99, and close #100 (its
fuzz_bootctlandfuzz_fw_updateread no input at all — see that review).
Not checked
- libFuzzer link and run: NOT RUN here. All six harnesses compile at this head; this host has no libFuzzer/ASan runtime (
libclang_rt.fuzzer.a,libclang_rt.asan.aabsent), so none links. TheFuzz Harness Buildcheck is green pergh pr checks 101, but I did not fetch the job log, so the claim that these harnesses have now been linked and run under libFuzzer rests on the check status and the author's comment, not on output I read. - The reported red-then-green history of this job: not reproduced. The off-by-one analysis on #99 is consistent with the code I read (
core/recovery.c:270bounds the last written byte; the oracle now matches atfuzz_recovery_protocol.c:50), but I did not run the crashing input under libFuzzer. - My reach-through numbers are not libFuzzer numbers. They come from a plain
gccdriver callingLLVMFuzzerTestOneInputin a loop over pseudo-random inputs. That is sound for finding 2 — a rejected erase is rejected regardless of how the input was chosen — and it is not a statement about what coverage-guided search would find. test_fuzz_harnesses.pywas run without pytest. No pytest on this host; I imported the module and called eachtest_*function directly. Same assertions, but I did not verify it under the runner CI uses, and codecov reports 4 lines/partials in that file uncovered.ctest: NOT RUN on this head. Thecore/ed25519_verify.c,include/eos_image.handtests/unit/test_ed25519.cparts of the diff are #94's commit (f704d87f), reviewed under #94/#98.- The local clone does not have
e2bb1f9f; everything above is from agh api tarballsnapshot at that sha.
Automated architecture review of e2bb1f9f5f6e — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
…resynchronising on garbage Answers the third review on embeddedos-org#84. Finding 3 (Low, and the one that matters on hardware) -- the header was read through a cast fdt_header_t* and direct member loads, while every struct-block read goes through the fdt_read_u32() memcpy helper precisely because the blob may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an odd offset: nothing promises 4-byte alignment, and on a strict-alignment cross target a direct member load is the same fault class this parser exists to avoid. The unit suite could never catch it -- blob_t.bytes happens to be aligned. All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains `undefined` so the fuzz job checks alignment too. Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the walk skipped 4 bytes and treated whatever followed as the next token, so a struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a TCB parser that walks garbage to completion is accepting input it does not understand. FDT_NOP -- the one legal unknown, padding the spec allows between tokens -- is now named in the header and passes; anything else returns -6. Findings 1, 2 and the -8 line: the fuzz-build job's single home is embeddedos-org#101 (nothing added here; the harness is inert until that job lands and then compiled by it -- said on the thread, not just here), and the header no longer documents return code -8, which nothing at this head returns. embeddedos-org#85 adds the -8 return and re-documents it together with the FDT_MAX_PATH_DEPTH move. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 16 tests PASS (was 13) discrimination, each fix in isolation: default: break restored -> test_a_garbage_tag_is_refused_not_skipped FAILS (expects -6, gets "not found") direct member load restored -> under UBSan the new unaligned-blob test reports "load of misaligned address 0x...671 for type 'const uint32_t'" at the exact line -- and cannot fire on the fixed code, which runs the same test clean NOP counter-check: interleaved FDT_NOP tokens still resolve, so the stricter default does not reject real trees. Refs embeddedos-org#84
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. #87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only #57's public_key_is_valid_subgroup() is wired to the call site, so #86's key_has_prime_order() was dead. Kept the live function, folded #86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on #86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight. * fix(fuzz): point every harness at the API it claims to fuzz Four of the five fuzz harnesses did not test what they name, and the build could not say so because not one of them includes a project header. Each declares its target itself: fuzz_recovery_protocol.c extern int eos_recovery_parse_packet(...) fuzz_fw_update.c extern int eos_fw_update_init(void) extern int eos_fw_update_process_chunk(...) extern int eos_fw_update_finalize(void) fuzz_bootctl.c extern int eos_bootctl_parse(...) fuzz_image_verify.c extern int eos_image_parse_header(const void *, size_t) eos_recovery_parse_packet, eos_fw_update_init, eos_fw_update_process_chunk and eos_bootctl_parse have never existed — those names appear nowhere outside the harness that declares them, so three of the five targets have never linked. The other two are worse, because they do link: - eos_fw_update_finalize exists but takes (ctx, mode), not (void). - eos_image_parse_header takes (uint32_t addr, eos_image_header_t *), not (const void *, size_t). fuzz_image_verify has been passing a pointer where an address is expected and a length where an output struct is expected, on every input, for as long as it has run. A local extern is why none of this was caught: it tells the compiler the symbol exists with whatever shape the harness asserts, and the mismatch survives to link time or past it. All five now include the real header and drive the real entry points: fuzz_image_verify parse_header -> verify_integrity, boot-path order fuzz_bootctl bootctl_load, then trap if load() accepted a block validate() rejects fuzz_fw_update begin -> write in fuzz-chosen chunk widths -> finalize or abort fuzz_recovery_protocol write_in_range, trapping on any accepted write that leaves the slot or wraps, checked in wider types that cannot fuzz_crypto already correct; switched to the header so it stays that way Adds tests/fuzz/fuzz_sim_flash.h: the three harnesses that reach flash need board ops installed, and without them they fuzz a null op table. That the compiler now checks this is not theoretical — writing this, it rejected EOS_UPGRADE_MODE_TEST for EOS_UPGRADE_TEST immediately, which is exactly the error class the externs were hiding. Verified. libFuzzer is unavailable here (libclang_rt.fuzzer_osx.a not found), so each harness was linked against eboot_core and driven by a standalone seed generator under ASan+UBSan: all five compile -Wall -Wextra 5/5, 0 failures 20,000 inputs each 100,000 total, no reports ctest 21/21 passed Third instance of this class across these repos, after eos#50 (fuzz_devicetree naming eos_dtb_parse) and eos#110 (fuzz_ota_header naming eos_ota_parse_header). The common factor every time is a fuzz target declaring its own subject. Stacked on #94, without which include/eos_image.h does not compile. * fix(fuzz): the recovery oracle was off by one, and CI proved it The restored Fuzz Harness Build job on #101 ran these harnesses under real libFuzzer for the first time and fuzz_recovery_protocol trapped within seconds. That was my bug, not the code's. The oracle computed the one-past-the-end address: end = base + offset + len; if (end > 0xFFFFFFFF) trap; The last byte a write of len bytes touches is base + offset + len - 1. eos_recovery_write_in_range() checks exactly that: if ((uint32_t)len - 1u > UINT32_MAX - (base + offset)) reject; So an accepted write whose final byte lands exactly on 0xFFFFFFFF -- base=0xFFFFFF00, offset=0, len=256 -- is legal, the function accepts it, and the old oracle trapped on it. A harness that traps on valid input reports the opposite of the truth. Verified both directions: the boundary case, fed directly -> no trap a fail-open stub accepting all input -> trap fires (exit 133) 500,000 random inputs, ASan+UBSan -> no reports libFuzzer doing precisely what it is for -- driving inputs into the one corner a hand-written oracle got wrong -- is the strongest argument yet for #101's job. The guard could never have caught this; only execution under a coverage-driven fuzzer did.
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. #87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only #57's public_key_is_valid_subgroup() is wired to the call site, so #86's key_has_prime_order() was dead. Kept the live function, folded #86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on #86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight. * fix(fdt): bound a device tree parser that was never compiled core/fdt_loader.c is in no source list, so it has never been built. The header offers it to callers, rtos_boot.c is named as the consumer, and nothing catches what is in it -- not the compiler, not ctest, not the sanitizer job. What is in it is a parser for a blob that comes out of flash, whose every header field it uses as an offset or a length without checking any of them. eos_fdt_validate() looks at magic and version only, so it accepts a blob whose off_dt_struct points anywhere: hdr.totalsize = 40 (the header alone) hdr.off_dt_struct = 0x100000 eos_fdt_validate(blob) -> 0 eos_fdt_get_prop(...) -> AddressSanitizer: BUS, READ at fdt_loader.c:25 in fdt_read_u32 Five more, all reachable the same way: * the tag read at the top of the loop takes 4 bytes where the loop condition guarantees 1; * strlen() on a node name reads until it finds a zero, which for a name running to the end of the block is past it; * nameoff indexes the strings block unchecked, so strcmp() reads from an arbitrary address; * a property len is clamped to the caller's buffer before the memcpy, which bounds the write but not the read -- an oversized len copies whatever follows the blob out to the caller; * FDT_END_NODE decrements depth with no floor. Bound them. validate() is the gate every path goes through, so the block offsets are checked against totalsize there, and get_prop() calls it before trusting the header. Inside the loop each read is checked for the width it takes, the name and property-name scans are bounded by memchr within their blocks, and the padded advances are re-checked for overrun. Adds the file to eboot_core and tests/unit/test_fdt_loader.c to ctest: ten cases, one well-formed tree that must still parse and nine malformed ones. Against the unfixed parser the suite fails on the third; with the bounds in place the whole suite is 20/20, and 20/20 under EBLDR_SANITIZE. Not fixed here, because it is a behaviour change rather than a safety one: node paths below the root do not resolve. _get_prop derives the depth to match from a slash count, so "/chosen" looks for depth 1, which is the root -- "chosen" is at depth 2. Only "/" resolves today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fdt): bound the parser against the caller's length, not the blob's claim Follow-up to the review on #84. Three findings from it, all in files this branch already owns. Finding 1 (High) -- every bound in eos_fdt_validate() was expressed relative to hdr->totalsize, which is read out of the same untrusted blob. Checking the block offsets against it proves the header is internally consistent and says nothing about how many bytes are really mapped: a 40-byte buffer declaring totalsize = 0x100000 with agreeing offsets passed every check, and eos_fdt_get_prop() then walked a megabyte past the allocation. The original reproducer (an inflated *offset* against an honest totalsize) was rejected; its mirror image was not. Adds the length-carrying entry points eos_fdt_validate_sized() and eos_fdt_get_prop_sized(), which take the bytes the caller actually owns and check totalsize against that before anything else. The existing two-argument forms stay as wrappers that trust the blob's own totalsize, documented in the header as a warrant the caller has to make good; eos_fdt_load() now passes max_size, which it had all along. Finding 2 (Medium) -- a property larger than the caller's buffer was copied short and reported as success, so a clipped value was indistinguishable from a complete one. In a boot path this reads bootargs. Now returns -7 and sets *buf_len to the full length so the caller can size a retry. Finding 3 (Medium) -- adds tests/fuzz/fuzz_fdt.c alongside the five existing harnesses. .ai/security.md names device tree among the parsers that get fuzz coverage rather than unit tests alone. It drives the sized entry points and passes the real size, which is what makes the harness meaningful -- the unsized forms would let a fuzzer authorise its own out-of-bounds read. Finding 6 (Low) -- drops the unreachable `total < sizeof(fdt_header_t)` check in eos_fdt_load(); validate() already rejects that blob, and the comment above it described a bound it was not applying. Header now documents the full -1..-8 return code table. Verified: cmake -DEBLDR_BUILD_TESTS=ON, ctest 21/21 PASS same under -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS test_fdt_loader 14 tests PASS (was 10) new tests against the pre-fix parser both FAIL as they should - inflated totalsize accepted at the sized entry point - truncating call returned 0 instead of -7 grep for eos_fdt_get_prop / eos_fdt_validate no callers outside the parser and its tests, so the -7 change breaks nothing today Refs #84 * fix(build): drop the duplicate core/fdt_loader.c registration master gained core/fdt_loader.c in eboot_core while this branch was open, so after the rebase it appeared twice and tests/unit/test_cmake_core_sources.py::test_core_sources_are_registered_once failed: AssertionError: ['core/fdt_loader.c'] is not false : duplicate eboot_core sources: ['core/fdt_loader.c'] Kept master's entry, dropped the one this branch added. That guard is the same shape as the one this stack adds for toolchain specs -- a build-file check that catches the class rather than the instance -- and it did its job. Verified: ctest 22/22 PASS, pytest 24 passed 1 skipped. Refs #84 * fix(fdt): one length-carrying API, and a CI job that builds the fuzz harnesses Answers the second review on #84. Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an out-of-bounds read by design. Both dereferenced hdr->totalsize before any length was known and handed that attacker-controlled value to the _sized form as its bound, so calling either on a short buffer read past it before a single check had run. The header called that a caller warrant; a warrant is not a check, and it is the exact bug this PR exists to fix. Neither had an in-tree caller. Removed rather than documented. There is now one form of each entry point and it always takes the length: int eos_fdt_validate(const void *fdt_blob, uint32_t avail); int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...); An exported unsafe twin in a TCB header is a future boot-path caller reintroducing the bug with no compiler complaint, which is worth more than the convenience of a one-argument call. eos_fdt_load() already passed max_size. The test that pinned the wrapper's behaviour is gone with the wrapper, and get_prop_sized_exact() collapsed into get_prop_exact() since they became the same function. Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ defaults OFF and no job set it, so the harness added here joined five others that no CI job compiles. A harness that is never built cannot fail to build, which is how fuzz_devicetree came to declare a function that did not exist and sit there unnoticed (eos#50). Adds a `fuzz-build` job: configure with clang, build every harness, and run each for five seconds over its own generated inputs. That is not a campaign -- it is enough to catch a harness that no longer compiles or crashes at once, which is the failure this repo has actually had. Note it needs EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt, so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all -- the job would have passed having compiled nothing. Found that locally before writing the job, not after. NOT RUN, and this is the honest limit: this host has no libFuzzer runtime (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link step cannot be reproduced here for any harness, old or new. What I verified is that CMake configures with both flags and reports all six targets -- "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update, fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this job is what makes them visible. Coordination note: #90 adds a `CI Gate` whose needs list is [test, build-arm, static-analysis]. Whichever of #84 and #90 lands second must add fuzz-build to that list -- and #90's own test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is not, which is the guard working rather than a trap. Finding 1 (High) is a PR-body correction, made there: the diff carries #94's master repair because this branch is stacked on it, and the body described only the FDT parser. Verified: ctest 22/22 PASS pytest tests/ 38 passed test_fdt_loader 13 tests PASS grep for a length-free entry point none remains in the header Refs #84 * ci: move the fuzz-harness build to its own PR The fuzz-build job added in the previous commit does its job -- it is red, on harnesses this PR did not write: undefined reference to `eos_fw_update_init' undefined reference to `eos_fw_update_process_chunk' undefined reference to `eos_recovery_parse_packet' None of those exist anywhere in the tree. tests/fuzz/fuzz_fw_update.c and fuzz_recovery_protocol.c declare an API that was never written, exactly as eos#50's fuzz_devicetree declared eos_dtb_parse(). They have compiled forever because nothing ever linked them. That is a real finding and it deserves a fix, not a red tick on an unrelated PR. The job and the repairs move to their own change; this PR keeps tests/fuzz/fuzz_fdt.c, which is the harness it is responsible for. Refs #84 * fix(fdt): read the header the way the struct block is read, and stop resynchronising on garbage Answers the third review on #84. Finding 3 (Low, and the one that matters on hardware) -- the header was read through a cast fdt_header_t* and direct member loads, while every struct-block read goes through the fdt_read_u32() memcpy helper precisely because the blob may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an odd offset: nothing promises 4-byte alignment, and on a strict-alignment cross target a direct member load is the same fault class this parser exists to avoid. The unit suite could never catch it -- blob_t.bytes happens to be aligned. All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains `undefined` so the fuzz job checks alignment too. Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the walk skipped 4 bytes and treated whatever followed as the next token, so a struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a TCB parser that walks garbage to completion is accepting input it does not understand. FDT_NOP -- the one legal unknown, padding the spec allows between tokens -- is now named in the header and passes; anything else returns -6. Findings 1, 2 and the -8 line: the fuzz-build job's single home is #101 (nothing added here; the harness is inert until that job lands and then compiled by it -- said on the thread, not just here), and the header no longer documents return code -8, which nothing at this head returns. #85 adds the -8 return and re-documents it together with the FDT_MAX_PATH_DEPTH move. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 16 tests PASS (was 13) discrimination, each fix in isolation: default: break restored -> test_a_garbage_tag_is_refused_not_skipped FAILS (expects -6, gets "not found") direct member load restored -> under UBSan the new unaligned-blob test reports "load of misaligned address 0x...671 for type 'const uint32_t'" at the exact line -- and cannot fire on the fixed code, which runs the same test clean NOP counter-check: interleaved FDT_NOP tokens still resolve, so the stricter default does not reject real trees. Refs #84 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
…85) * fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. #87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only #57's public_key_is_valid_subgroup() is wired to the call site, so #86's key_has_prime_order() was dead. Kept the live function, folded #86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on #86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight. * fix(fdt): bound a device tree parser that was never compiled core/fdt_loader.c is in no source list, so it has never been built. The header offers it to callers, rtos_boot.c is named as the consumer, and nothing catches what is in it -- not the compiler, not ctest, not the sanitizer job. What is in it is a parser for a blob that comes out of flash, whose every header field it uses as an offset or a length without checking any of them. eos_fdt_validate() looks at magic and version only, so it accepts a blob whose off_dt_struct points anywhere: hdr.totalsize = 40 (the header alone) hdr.off_dt_struct = 0x100000 eos_fdt_validate(blob) -> 0 eos_fdt_get_prop(...) -> AddressSanitizer: BUS, READ at fdt_loader.c:25 in fdt_read_u32 Five more, all reachable the same way: * the tag read at the top of the loop takes 4 bytes where the loop condition guarantees 1; * strlen() on a node name reads until it finds a zero, which for a name running to the end of the block is past it; * nameoff indexes the strings block unchecked, so strcmp() reads from an arbitrary address; * a property len is clamped to the caller's buffer before the memcpy, which bounds the write but not the read -- an oversized len copies whatever follows the blob out to the caller; * FDT_END_NODE decrements depth with no floor. Bound them. validate() is the gate every path goes through, so the block offsets are checked against totalsize there, and get_prop() calls it before trusting the header. Inside the loop each read is checked for the width it takes, the name and property-name scans are bounded by memchr within their blocks, and the padded advances are re-checked for overrun. Adds the file to eboot_core and tests/unit/test_fdt_loader.c to ctest: ten cases, one well-formed tree that must still parse and nine malformed ones. Against the unfixed parser the suite fails on the third; with the bounds in place the whole suite is 20/20, and 20/20 under EBLDR_SANITIZE. Not fixed here, because it is a behaviour change rather than a safety one: node paths below the root do not resolve. _get_prop derives the depth to match from a slash count, so "/chosen" looks for depth 1, which is the root -- "chosen" is at depth 2. Only "/" resolves today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fdt): bound the parser against the caller's length, not the blob's claim Follow-up to the review on #84. Three findings from it, all in files this branch already owns. Finding 1 (High) -- every bound in eos_fdt_validate() was expressed relative to hdr->totalsize, which is read out of the same untrusted blob. Checking the block offsets against it proves the header is internally consistent and says nothing about how many bytes are really mapped: a 40-byte buffer declaring totalsize = 0x100000 with agreeing offsets passed every check, and eos_fdt_get_prop() then walked a megabyte past the allocation. The original reproducer (an inflated *offset* against an honest totalsize) was rejected; its mirror image was not. Adds the length-carrying entry points eos_fdt_validate_sized() and eos_fdt_get_prop_sized(), which take the bytes the caller actually owns and check totalsize against that before anything else. The existing two-argument forms stay as wrappers that trust the blob's own totalsize, documented in the header as a warrant the caller has to make good; eos_fdt_load() now passes max_size, which it had all along. Finding 2 (Medium) -- a property larger than the caller's buffer was copied short and reported as success, so a clipped value was indistinguishable from a complete one. In a boot path this reads bootargs. Now returns -7 and sets *buf_len to the full length so the caller can size a retry. Finding 3 (Medium) -- adds tests/fuzz/fuzz_fdt.c alongside the five existing harnesses. .ai/security.md names device tree among the parsers that get fuzz coverage rather than unit tests alone. It drives the sized entry points and passes the real size, which is what makes the harness meaningful -- the unsized forms would let a fuzzer authorise its own out-of-bounds read. Finding 6 (Low) -- drops the unreachable `total < sizeof(fdt_header_t)` check in eos_fdt_load(); validate() already rejects that blob, and the comment above it described a bound it was not applying. Header now documents the full -1..-8 return code table. Verified: cmake -DEBLDR_BUILD_TESTS=ON, ctest 21/21 PASS same under -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS test_fdt_loader 14 tests PASS (was 10) new tests against the pre-fix parser both FAIL as they should - inflated totalsize accepted at the sized entry point - truncating call returned 0 instead of -7 grep for eos_fdt_get_prop / eos_fdt_validate no callers outside the parser and its tests, so the -7 change breaks nothing today Refs #84 * fix(build): drop the duplicate core/fdt_loader.c registration master gained core/fdt_loader.c in eboot_core while this branch was open, so after the rebase it appeared twice and tests/unit/test_cmake_core_sources.py::test_core_sources_are_registered_once failed: AssertionError: ['core/fdt_loader.c'] is not false : duplicate eboot_core sources: ['core/fdt_loader.c'] Kept master's entry, dropped the one this branch added. That guard is the same shape as the one this stack adds for toolchain specs -- a build-file check that catches the class rather than the instance -- and it did its job. Verified: ctest 22/22 PASS, pytest 24 passed 1 skipped. Refs #84 * fix(fdt): one length-carrying API, and a CI job that builds the fuzz harnesses Answers the second review on #84. Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an out-of-bounds read by design. Both dereferenced hdr->totalsize before any length was known and handed that attacker-controlled value to the _sized form as its bound, so calling either on a short buffer read past it before a single check had run. The header called that a caller warrant; a warrant is not a check, and it is the exact bug this PR exists to fix. Neither had an in-tree caller. Removed rather than documented. There is now one form of each entry point and it always takes the length: int eos_fdt_validate(const void *fdt_blob, uint32_t avail); int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...); An exported unsafe twin in a TCB header is a future boot-path caller reintroducing the bug with no compiler complaint, which is worth more than the convenience of a one-argument call. eos_fdt_load() already passed max_size. The test that pinned the wrapper's behaviour is gone with the wrapper, and get_prop_sized_exact() collapsed into get_prop_exact() since they became the same function. Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ defaults OFF and no job set it, so the harness added here joined five others that no CI job compiles. A harness that is never built cannot fail to build, which is how fuzz_devicetree came to declare a function that did not exist and sit there unnoticed (eos#50). Adds a `fuzz-build` job: configure with clang, build every harness, and run each for five seconds over its own generated inputs. That is not a campaign -- it is enough to catch a harness that no longer compiles or crashes at once, which is the failure this repo has actually had. Note it needs EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt, so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all -- the job would have passed having compiled nothing. Found that locally before writing the job, not after. NOT RUN, and this is the honest limit: this host has no libFuzzer runtime (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link step cannot be reproduced here for any harness, old or new. What I verified is that CMake configures with both flags and reports all six targets -- "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update, fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this job is what makes them visible. Coordination note: #90 adds a `CI Gate` whose needs list is [test, build-arm, static-analysis]. Whichever of #84 and #90 lands second must add fuzz-build to that list -- and #90's own test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is not, which is the guard working rather than a trap. Finding 1 (High) is a PR-body correction, made there: the diff carries #94's master repair because this branch is stacked on it, and the body described only the FDT parser. Verified: ctest 22/22 PASS pytest tests/ 38 passed test_fdt_loader 13 tests PASS grep for a length-free entry point none remains in the header Refs #84 * ci: move the fuzz-harness build to its own PR The fuzz-build job added in the previous commit does its job -- it is red, on harnesses this PR did not write: undefined reference to `eos_fw_update_init' undefined reference to `eos_fw_update_process_chunk' undefined reference to `eos_recovery_parse_packet' None of those exist anywhere in the tree. tests/fuzz/fuzz_fw_update.c and fuzz_recovery_protocol.c declare an API that was never written, exactly as eos#50's fuzz_devicetree declared eos_dtb_parse(). They have compiled forever because nothing ever linked them. That is a real finding and it deserves a fix, not a red tick on an unrelated PR. The job and the repairs move to their own change; this PR keeps tests/fuzz/fuzz_fdt.c, which is the harness it is responsible for. Refs #84 * fix(fdt): read the header the way the struct block is read, and stop resynchronising on garbage Answers the third review on #84. Finding 3 (Low, and the one that matters on hardware) -- the header was read through a cast fdt_header_t* and direct member loads, while every struct-block read goes through the fdt_read_u32() memcpy helper precisely because the blob may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an odd offset: nothing promises 4-byte alignment, and on a strict-alignment cross target a direct member load is the same fault class this parser exists to avoid. The unit suite could never catch it -- blob_t.bytes happens to be aligned. All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains `undefined` so the fuzz job checks alignment too. Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the walk skipped 4 bytes and treated whatever followed as the next token, so a struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a TCB parser that walks garbage to completion is accepting input it does not understand. FDT_NOP -- the one legal unknown, padding the spec allows between tokens -- is now named in the header and passes; anything else returns -6. Findings 1, 2 and the -8 line: the fuzz-build job's single home is #101 (nothing added here; the harness is inert until that job lands and then compiled by it -- said on the thread, not just here), and the header no longer documents return code -8, which nothing at this head returns. #85 adds the -8 return and re-documents it together with the FDT_MAX_PATH_DEPTH move. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 16 tests PASS (was 13) discrimination, each fix in isolation: default: break restored -> test_a_garbage_tag_is_refused_not_skipped FAILS (expects -6, gets "not found") direct member load restored -> under UBSan the new unaligned-blob test reports "load of misaligned address 0x...671 for type 'const uint32_t'" at the exact line -- and cannot fire on the fixed code, which runs the same test clean NOP counter-check: interleaved FDT_NOP tokens still resolve, so the stricter default does not reject real trees. Refs #84 * fix(fdt): resolve a node by its path, not by its depth and last name Replaces slash-counting and last-component matching with a component walk gated on ancestor matches, so /soc/uart and /decoy/uart are distinguishable. Also carries the fixes from the review on this PR: Finding 1 (High) -- the property arm tested in_target alone, and in_target is cleared only by the target's own FDT_END_NODE, so it stayed true for the entire subtree and a property found on a *child* was returned as the target's own. On this PR's own decoy tree, eos_fdt_get_prop(blob, "/soc", "reg", ...) returned 0 with "soc-uart" when /soc has no reg property at all and the correct answer is -5. Real device trees emit properties before subnodes, so a property that is present on the target is still found first -- the bug bites when the target lacks it, turning "not found" into a silently wrong value from a nested node. For an attacker-supplied blob the ordering is not enforced at all. Fixed by also requiring depth == target_depth, which is exact: depth is target_depth for the target's own properties and target_depth + 1 or deeper inside any child. Finding 2 (Low) -- test_the_parent_of_the_node_has_to_match_too asserted a second positive resolution rather than a rejection, so the suite read as though it had a negative case for wrong parents when the negative coverage actually lives in test_a_node_below_the_root_resolves. Renamed to test_each_uart_returns_its_own_value, and the load-bearing property (decoy is emitted before soc, so a last-component matcher would return "decoy-uart") is now stated on the test that depends on it. Finding 3 (Low) -- a path deeper than FDT_MAX_PATH_DEPTH returned -1, the same code as a NULL argument, so a caller could not tell a programming error from malformed input. Now -8, documented in the header's code table. Verified: ctest 21/21 PASS same under -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS test_fdt_loader 20 tests PASS (was 14) test_a_property_on_a_child_is_not_returned_as_the_parents, run against this same parser with only the depth guard reverted: [FAIL] get_prop_exact(&b, "/soc", "reg", out, &len) != 0 so it discriminates rather than restating the fix. Refs #85 * fix(fdt): match node names past the unit address, where real device trees put it Replaces slash-counting and last-component matching with a component walk gated on ancestor matches, and matches a component against the node name up to its unit address -- `/soc/uart` resolves `uart@40011000`, an explicit `@40011000` still selects exactly that node, and an address not in the tree does not fall back to a loose match. Carries the earlier review fixes (depth == target_depth on the property arm; FDT_MAX_PATH_DEPTH moved to the public header; -8 for an over-deep path, distinct from -1) and, from the overnight round: - the stray `/* Deepest node path ... */` comment left in the .c after the define moved to the header is gone (Low) - return code -8 is documented in the header's code table again, on this branch, because this is the branch where anything returns it -- #84 dropped the line for exactly that reason Rebased onto #84's alignment/NOP round; the test-file merge keeps both sides' suites (garbage-tag, NOP-padding and unaligned-blob from #84; the node-path and unit-address suites from here). Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 24 tests PASS Refs #85 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/scorecard-action@v2.4.0 pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.
…resynchronising on garbage Answers the third review on embeddedos-org#84. Finding 3 (Low, and the one that matters on hardware) -- the header was read through a cast fdt_header_t* and direct member loads, while every struct-block read goes through the fdt_read_u32() memcpy helper precisely because the blob may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an odd offset: nothing promises 4-byte alignment, and on a strict-alignment cross target a direct member load is the same fault class this parser exists to avoid. The unit suite could never catch it -- blob_t.bytes happens to be aligned. All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains `undefined` so the fuzz job checks alignment too. Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the walk skipped 4 bytes and treated whatever followed as the next token, so a struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a TCB parser that walks garbage to completion is accepting input it does not understand. FDT_NOP -- the one legal unknown, padding the spec allows between tokens -- is now named in the header and passes; anything else returns -6. Findings 1, 2 and the -8 line: the fuzz-build job's single home is embeddedos-org#101 (nothing added here; the harness is inert until that job lands and then compiled by it -- said on the thread, not just here), and the header no longer documents return code -8, which nothing at this head returns. embeddedos-org#85 adds the -8 return and re-documents it together with the FDT_MAX_PATH_DEPTH move. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 16 tests PASS (was 13) discrimination, each fix in isolation: default: break restored -> test_a_garbage_tag_is_refused_not_skipped FAILS (expects -6, gets "not found") direct member load restored -> under UBSan the new unaligned-blob test reports "load of misaligned address 0x...671 for type 'const uint32_t'" at the exact line -- and cannot fire on the fixed code, which runs the same test clean NOP counter-check: interleaved FDT_NOP tokens still resolve, so the stricter default does not reject real trees. Refs embeddedos-org#84
Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/scorecard-action@v2.4.0 pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.
Stacked on #99. #99 repairs the five harnesses in
tests/fuzz/; this stops thesame class coming back, in the two places it can.
Why it came back-able
Nothing compiles them.
tests/fuzz/is behindEBLDR_BUILD_FUZZ, whichdefaults
OFF, and no job set it. That is the whole reason three harnessescould name functions that exist nowhere in the repository —
eos_bootctl_parse,eos_recovery_parse_packet,eos_fw_update_initandfriends — and sit there unnoticed. A harness that is not built cannot fail to
build.
The
externis still legal. All four defects came from a hand-writtenprototype standing in for an
#include. Anexternis a promise the compileris obliged to believe and has no way to check, which is why the fourth was
invisible even to a compiler:
fuzz_image_verifydeclaredagainst a real
int eos_image_parse_header(uint32_t addr, eos_image_header_t *out).It linked. It ran. It fuzzed nothing.
What this adds
fuzz-buildjob (Fuzz Harness Build)EBLDR_BUILD_FUZZ=ON, then smoke-runs each for five seconds.EBLDR_BUILD_TESTS=ONtoo —tests/fuzzis added fromtests/CMakeLists.txt, so the fuzz option alone builds nothing and the job would pass having compiled no harness at all. It does not fuzz; a campaign belongs in a scheduled workflow.tests/unit/test_fuzz_harnesses.pyeboot_core, definesLLVMFuzzerTestOneInput, and declares no prototypes of its own.The two halves catch different failures and neither subsumes the other: the
guard is static and stops the pattern being written at all; the job is a
compiler, and catches a signature that drifts under a harness that already
includes the right header.
Mutation-checked:
externprototypetest_no_harness_declares_its_own_prototypesadd_executabletest_every_harness_is_builtThe job paid for itself on its first run
Not a hypothetical. The first time these harnesses ran under real libFuzzer —
this job, on this PR —
fuzz_recovery_protocoltrapped in six seconds ona write covering
[0xFFFFDBDB, 0xFFFFFFFF]: 9253 bytes ending on the lastaddressable byte, every one of them inside 32 bits, so accepting it is correct.
The harness's oracle was asserting on
base + offset + len > 0xFFFFFFFF— theaddress one past the end — where
eos_recovery_write_in_range()correctlybounds the last byte written, exactly as its own final line says:
if ((uint32_t)len - 1u > UINT32_MAX - (base + offset)). The function wasright and the harness reported the opposite. Fixed in #99 as
bbf4e89.That is the case for keeping both halves. The static guard could never have
found an off-by-one inside an oracle — it is a real
#includecalling a realfunction with the right types. Only executing it under a coverage-driven fuzzer
could, and until this job existed nothing did.
Verification
pytest tests/unitctestOn this head the
Fuzz Harness Buildjob is green in CI, along with theother 24 checks.
Locally the job could not be run: Apple clang ships no
libclang_rt.fuzzer, so-fsanitize=fuzzerwill not link on this host. Each of #99's five harnesses wasinstead compiled and linked against
eboot_corewith a stand-in driver under-fsanitize=address,undefinedand run over ~20k pseudo-random inputs plus everylength from 0 to 200 — all five clean. That covers symbol resolution and harness
logic but not the libFuzzer link or its coverage-guided mutation, which is
precisely the gap that found the oracle bug above. CI is the only place that
runs.
Merge order
After #99 (head
bbf4e89), which is after #94 —masterdoes not compilewithout it. Nothing here touches
tests/fuzz/*.c, so there is no overlap with#99's diff.